-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Model il #138
Conversation
* moved test_cash_out_accuracy from omnipool_amm to omnipool_state * in test_cash_out_accuracy, give agent shares and holdings in every asset
* Added limit for add liquidity in test_add_manipulation * Excluding price_move of 0 in test_withdraw_manipulation * lift trade cap so arb trades don't fail --------- Co-authored-by: jepidoptera <jepidoptera@live.com>
* Removed cadCAD * Removed cadCAD directory * Removed cadCAD from requirements.txt
remove extra balance check from execute_swap
… it is not in scope for this PR
…to add-price_manipulation_tests
Test invest all
Fix test lp limits
* Adding tests - WIP * Adding test for minimum withdrawal fee * Added nonzero minimums to price_move and lp_percent so that test would not fail with these in the range of 1e-300 * Added test for dynamic withdrawal fee * Added test of remove_liquidity when the LP price and the current price are different * Removed redundant setting of oracle values in omnipool_config * Added error message to test_remove_liquidity_min_fee * Simplified code to set last oracle values in test_remove_liquidity_dynamic_fee * Removed unused import * Added error messages * Formatting
…fference in price
"# price_list = processing.import_binance_prices(['BTC', 'ETH', 'DOT'], start_date='Jan 1 2023', days = 120)\n", | ||
"\n", | ||
"assets = {\n", | ||
" 'HDX': {'usd price': 0.05, 'weight': 0.7999},\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HDX plays a special role in the Omnipool currently, with LRNA fees being directed to the HDX pool whenever imbalance = 0. I'd thus prefer HDX weight to be realistic (5% or so?) and for there to be some other generic token to make up the additional 65% of the pool.
Would this wind up being identical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this notebook, it's the weight of USD and not HDX that changes, and the only thing that changes in price is TKN. I changed it to make that clearer and also updated the weights so USD is the majority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HDX/USD price shouldn't change in this notebook, right? Is that what we see? I would expect there needs to be some trade involving HDX to make this happen, which would change its weight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my question above stands... and I see that we have TKN and TKN2 in both notebooks? That's what I believe is appropriate.
hydradx/notebooks/Omnipool/vault/ImpermanentLossAnalysis2.ipynb
Outdated
Show resolved
Hide resolved
…rity with the other notebook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something is off in ImpermanentLossAnalysis.ipynb between the initial instantiation of the Omnipool and the instantiation in the for look. TKN2 is initially in the pool, but isn't present in the for loop. It's not clear to me which is correct.
In ImpermanentLossAnalysis2.ipynb, I think we need HDX to move with the market too. This may mean that I was wrong to suggest that we need HDX and TKN2 to be split out separately, although I still think this is actually clearer. I would simply add HDX to the "other_assets" so that it moves with the market.
"# price_list = processing.import_binance_prices(['BTC', 'ETH', 'DOT'], start_date='Jan 1 2023', days = 120)\n", | ||
"\n", | ||
"assets = {\n", | ||
" 'HDX': {'usd price': 0.05, 'weight': 0.7999},\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my question above stands... and I see that we have TKN and TKN2 in both notebooks? That's what I believe is appropriate.
…he first ImpermanentLossAnalysis notebook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one more change is needed in ImpermanentLossAnalysis2.ipynb
* Notebooks which illustrate IL exposure in Omnipool --------- Co-authored-by: poliwop <grove.colin@gmail.com>
No description provided.